Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support java-test-fixtures plugin #223

Merged
merged 2 commits into from
Aug 18, 2020
Merged

Conversation

pkubowicz
Copy link
Contributor

@pkubowicz pkubowicz commented Jul 27, 2020

Applying 'java-test-fixtures' plugin caused 'No mutations found' from Pitest.

The plugin changes test runtime classpath: tested code is included as a JAR instead of as a directory
(see gradle/gradle#11696). It seems that Pitest ignores correct directories being passed using --mutableCodePaths and insists on having those directories also passed in --classPath.

Also change functional tests: runTasksSuccessfully() fails the test without giving any context, so it should be avoided.


The project I work on uses java-test-fixtures plugin, so this issue blocks me from using Pitest.

@pkubowicz
Copy link
Contributor Author

pkubowicz commented Jul 27, 2020

Builds failing on Travis seems to be victims of the unstable build. My PR passes on Travis for Java 11 and 14. Tests pass when I run them locally with Java 8. AppVeyor build with Java 8 passes.

Copy link
Owner

@szpak szpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR.

The build fails only with Java 8, as PIT 1.1.5 is not treated as Java 11 compatible and:

PitestPluginPitVersionFunctionalSpec.setup and run pitest task with PIT 1.1.5 FAILED

isn't executed in the other builds. As PIT 1.4.0 was the one to required Java 8, feel free to remove any older PIT versions from the verification matrix.

I put some comment/questions/suggestions inline in code.

@@ -21,8 +23,10 @@ class Junit5FunctionalSpec extends AbstractPitestFunctionalSpec {
given:
copyResources("testProjects/junit5kotlin", "")
when:
ExecutionResult result = runTasksSuccessfully('pitest', '-b', 'build.gradle.kts')
ExecutionResult result = runTasks('pitest')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy-paste? You probably would like to build using the kotlin build file :).

@CompileDynamic
class TestFixturesFunctionalSpec extends AbstractPitestFunctionalSpec {

void "should work with kotlin and test fixtures"() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kotlin and test fixtures? That project seem to have classes written in Java and build file in Groovy :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a copy-paste mistake, I will fix it.

task.additionalClasspath.setFrom({
List<FileCollection> testRuntimeClasspath = (extension.testSourceSets.get() as Set<SourceSet>)*.runtimeClasspath
FileCollection combinedTaskClasspath = project.objects.fileCollection().from(testRuntimeClasspath)
FileCollection filteredCombinedTaskClasspath = combinedTaskClasspath.filter { File file ->
!extension.fileExtensionsToFilter.getOrElse([]).find { extension -> file.name.endsWith(".$extension") }
}
!extension.fileExtensionsToFilter.getOrElse([]).find { extension -> file.name.endsWith(".$extension") || file == "core.jar" }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why core.jar is special?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to remove it after debugging a local project.

return [new File(project.buildDir, "classes//java//${sourceSetName}"),
new File(project.buildDir, "resources//${sourceSetName}")
return [new File(project.buildDir, "classes/java/${sourceSetName}"),
new File(project.buildDir, "resources/${sourceSetName}")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that I had some problems (somewhere) with just a single / on Windows and // was fine on both *Unix and Windows. However, the tests passed on AppVeyor, so it doesn't seem to be a problem here...

task.taskArgumentMap()['classPath'] ==
(
assembleSourceSetsClasspathByNameAsStringSet("intTest") +
[new File(project.buildDir, "classes/java/main").absolutePath]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder that it started to be needed here. classpath didn't contain have classes/java/main before, assuming java plugin was applied? Or it was a limitation of this "unit" test (and in real execution it was included)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intTest source set is created from scratch and does not have any relation to main source set, so intTest classpath does not include any classes from main. I suppose previously such a project won't work with real Pitest, because Pitest would fail to find classes to mutate.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know what confused me.

 extension.mainSourceSets.set([javaSourceSets.getByName(SourceSet.MAIN_SOURCE_SET_NAME)])

and:

    private Set<File> calculateBaseMutableCodePaths() {
        Set<SourceSet> sourceSets = extension.mainSourceSets.get()
        return sourceSets*.output.classesDirs.files.flatten() as Set<File>
    }

which you call should cover it. However, in the past it was used only for mutableCodePaths, not for (additional)classpath, what you fixed making it java-test-fixtures plugin compatible :).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, could you, making other changes, refactor assembleSourceSetsClasspathByNameAsStringSet to generate class only or class+resources classpath to keep new File(project.buildDir, "classes/java/XXX") in one place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you wanted, but I changed the path handling.

Please check and close other discussions, I think I addressed all your comments.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of :). I will tune it up after merge.

@szpak
Copy link
Owner

szpak commented Aug 2, 2020

Btw, I spotted that the weekly cron build on Travis hasn't started "recently" and the problem with PIT 1.1.5 started to occur also in devel.

image

I will fix it right away in devel and let you know to rebase your branch.

szpak added a commit that referenced this pull request Aug 2, 2020
Due to integration test failures with recent Java 8 versions:
#223 (review)
@szpak
Copy link
Owner

szpak commented Aug 2, 2020

Building. Feel free to rebase once it's green.

Applying 'java-test-fixtures' plugin caused 'No mutations found'
from Pitest.

The plugin changes test runtime classpath:
tested code is included as a JAR instead of as a directory
(see gradle/gradle#11696). It seems that Pitest ignores correct
directories being passed using --mutableCodePaths and insists on
having those directories also passed in --classPath.

Also change functional tests: runTasksSuccessfully() fails the test
without giving any context, so it should be avoided.
@pkubowicz
Copy link
Contributor Author

Is there something this PR is waiting for to be merged?

@szpak
Copy link
Owner

szpak commented Aug 5, 2020

Nope, I accepted it and it's waiting for a longer moment of time to adjust the last commit after merge :).

@pkubowicz
Copy link
Contributor Author

Can you release this bugfix and refactor tests later? There is no workaround for this problem.

@szpak szpak added this to the 1.5.2 milestone Aug 18, 2020
@szpak szpak merged commit 0747c03 into szpak:master Aug 18, 2020
@szpak
Copy link
Owner

szpak commented Aug 18, 2020

Thanks for a reminder. 1.5.2 is just being released on Travis:
https://travis-ci.org/github/szpak/gradle-pitest-plugin/jobs/719031199

@pkubowicz pkubowicz deleted the test-fixtures branch August 18, 2020 18:21
@pkubowicz
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants